Skip to content

allowing ID randomization but keeping the stable selector #729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

allowing ID randomization but keeping the stable selector #729

wants to merge 1 commit into from

Conversation

PardeepSinghBali
Copy link
Contributor

@PardeepSinghBali PardeepSinghBali commented Mar 18, 2025 •
edited
Loading

What does this change?

This PR addresses an issue related to form container ID randomization for improved accessibility and Address Dropdown not populating the addresses at drupal Form. Previously, the localgov_base_preprocess_container() function was appending a random suffix to the edit-actions form container ID. This introduced potential issues with JavaScript selectors that rely on the #edit-actions ID, as they were no longer able to target the element due to the dynamic nature of the ID.

This change ensures that:

  1. The ID is randomized for accessibility, but we still maintain a stable selector (data-drupal-selector) to be used by JavaScript.
  2. JavaScript selectors that depend on the original ID (edit-actions) will work as expected, avoiding potential breakage in behavior.
  3. The random ID is applied to the id attribute, but the original ID is retained as data-drupal-selector for stable targeting.

How to test

Check the edit-actions form container element:

On the main branch, inspect the container and check the id attribute. It should be static (e.g., #edit-actions).
On this branch, inspect the same container and verify that the id has been randomized (e.g., #edit-actions--) but the data-drupal-selector remains as edit-actions.

How can we measure success?

  1. No errors or broken JavaScript functionality related to the #edit-actions container ID after the change.
  2. JavaScript selectors that use data-drupal-selector="edit-actions" work as expected.
  3. Improved accessibility with randomized form container IDs while keeping the stable selector intact.

Have we considered potential risks?

Images

Accessibility

@andybroomfield andybroomfield self-requested a review March 18, 2025 17:46
@siliconmeadow siliconmeadow self-requested a review March 19, 2025 10:23
@siliconmeadow
Copy link
Member

This is in reference #325, btw.

Adnan-cds
Adnan-cds reviewed Mar 19, 2025
View reviewed changes
Copy link
Contributor

@Adnan-cds Adnan-cds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this change in a webform with two address lookup elements and it worked okay.

I would request Pardeep to rename the Git branch though. I am linking to the relevant LocalGov Drupal documentation.

Mark or Maria, would you be able to test this as well? @markconroy @msayoung

@Adnan-cds
Copy link
Contributor

Please also fix the coding standards errors. This should be easy: $ cd /path/to/your/localgov/site/; ./bin/phpcbf web/themes/contrib/localgov_base/localgov_base.theme

@siliconmeadow
Copy link
Member

siliconmeadow commented Mar 19, 2025 •
edited
Loading

@Adnan-cds - I'm trying to help @PardeepSinghBali out and we're unable to rename the branch here on github. He's renamed it locally and pushed it to GH and it looks like he'll have to created yet another PR using that branch instead. Are you, or someone with greater privileges able to change the branch on this PR? What's the best way to proceed? This is the branch which has the CS fixes, btw: fix/1.x-325-fix-RandomID.

@andybroomfield
Copy link
Contributor

Have also tested this with an address field in localgov_scarfolk and the address list is populating. This is on a release version of localgov_forms (beta9) so it doesn't have the previously merged code from localgovdrupal/localgov_forms#110

Localgov.scarfolk.address.lookup.fix.mp4

@PardeepSinghBali
Copy link
Contributor Author

Have also tested this with an address field in localgov_scarfolk and the address list is populating. This is on a release version of localgov_forms (beta9) so it doesn't have the previously merged code from localgovdrupal/localgov_forms#110

Localgov.scarfolk.address.lookup.fix.mp4

we dont need this js change anymore [localgovdrupal/localgov_forms#110]

@ekes ekes closed this Mar 19, 2025
@ekes ekes deleted the PS_resolvingRandomID branch March 19, 2025 12:25
@Adnan-cds
Copy link
Contributor

we're unable to rename the branch here on github.

Could you please attempt a rename from https://github.com/localgovdrupal/localgov_base/branches. Github has some instructions. If this doesn't work, I can try it on your behalf.

@PardeepSinghBali
Copy link
Contributor Author

PardeepSinghBali commented Mar 19, 2025 •
edited
Loading

I can see this PR is Closed. better idea is i am creating a new PR
#731

@PardeepSinghBali PardeepSinghBali mentioned this pull request Mar 19, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Adnan-cds Adnan-cds Adnan-cds left review comments

@ekes ekes Awaiting requested review from ekes

@agile-simon agile-simon Awaiting requested review from agile-simon

@andybroomfield andybroomfield Awaiting requested review from andybroomfield

@siliconmeadow siliconmeadow Awaiting requested review from siliconmeadow

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@PardeepSinghBali @siliconmeadow @Adnan-cds @andybroomfield @ekes